fix: resolve and validate the output format once, in the root command - #680

Bug: this write-back flips task wait into a different output mode when no -f was given.

pflag.FlagSet.Set marks the flag Changed = true (pflag v1.0.10, flag.go:509), and the flag object is shared with every subcommand's flag set. pkg/cmd/task/wait/wait.go:333 uses Changed to mean "the user explicitly asked for a format":

isFormatSpecified := opts.Command.Flags().Changed(constants.FlagOutputFormat)

Before this PR that was false for a plain octopus task wait <id>, so the legacy progress formatter ran. After this PR the root pre-run always calls Set, so isFormatSpecified is always true and (with the resolved default table) shouldUseCustomOutputFormat returns true — plain task wait silently switches to the custom table output.

Writing through the Value directly updates the value without touching Changed, exactly like the alias-copy loop above:

Actioned in 94c1a72, with a regression test added in dd307dc.

The claim holds. shouldUseCustomOutputFormat (pkg/cmd/task/wait/wait.go:333) reads Changed(FlagOutputFormat) as "the user asked for a format", and cmd.PersistentFlags() hands the same *pflag.Flag to every subcommand, so one FlagSet.Set in the root pre-run makes that true for every invocation. With the resolved default table, isJsonOrTable is true as well, so a plain octopus task wait <id> would have dropped the legacy progress formatter.

Took the suggestion as written — cmdPFlags.Lookup(constants.FlagOutputFormat).Value.Set(outputFormat) — which matches the alias-copy loop directly above it and leaves Changed alone.

Covered by TestNewCmdRoot_PreRunDoesNotMarkTheOutputFormatFlagAsChanged in pkg/cmd/root/root_test.go. It drives the real PersistentPreRunE rather than calling resolveOutputFormat in isolation, because the in-isolation tests can't see this at all, and asserts the flag comes out valued table with Changed false on both the new and the legacy flag. I checked it pins the bug rather than the shape of the fix: swapping the line back to cmdPFlags.Set(...) fails it at root_test.go:138 with Should be false.

Bug: an invalid OutputFormat config value now soft-bricks the whole CLI, including the command that would fix it.

octopus config set OutputFormat xml succeeds today — pkg/cmd/config/set/set.go validates only the key (and NoPrompt's bool), never the OutputFormat value, and the interactive prompt accepts free text. Once that value is in the config file, this pre-run returns a usage error for every invocation: octopus config set OutputFormat table, octopus config list, octopus help, octopus --version, and shell completion (__complete) all die before their RunE runs. The only ways out are hand-editing the config file or guessing the non-obvious escape hatch of appending an explicit -f table (which wins via Changed).

Two complementary fixes:

• validate the value in config set with the new constants.IsValidOutputFormat (write-side), and
• for a config-sourced (as opposed to flag-sourced) invalid value, warn and fall back to the default instead of hard-failing, so a bad file never locks the user out.

Actioned in 883e7ef, with a regression test added in dd307dc. Both halves, as suggested:

• Write side — setRun now has a case strings.ToLower(constants.ConfigOutputFormat) alongside the NoPrompt one, which lowercases/trims and rejects anything constants.IsValidOutputFormat doesn't know before localViper.Set. The interactive path lands in the same place: promptMissing only fills value in, then falls through to the same switch, so free text typed at the prompt is rejected too.
• Read side — resolveOutputFormat treats a bad config-sourced value as a warning on stderr and carries on down the precedence chain, so an already-poisoned file degrades to the default. A bad value from the flag is still a hard usage error, since that one the user just typed.

I built the binary and ran it against a config file containing "OutputFormat": "xml", with HOME pointed at a scratch dir, walking every escape route named above:

$ octopus --version                     -> 2.23.10                          exit 0
$ octopus help                          -> help text                        exit 0
$ octopus config list                   -> table of config                  exit 0
$ octopus __complete config set ""      -> ":0 / ShellCompDirectiveDefault"  exit 0
$ octopus config get OutputFormat       -> xml                              exit 0
$ octopus config set OutputFormat table --no-prompt                         exit 0
    config file afterwards: "outputformat": "table"
$ octopus config list                   -> no warning, table                exit 0

Each of the first six printed Ignoring the OutputFormat config setting: unsupported output format 'xml'. Valid values are 'json', 'table', 'basic' and then did its job, so the fix-it command runs without needing the -f table escape hatch, and once it has run the warning stops.

Three things I checked rather than assumed, since "can't lock the user out" is the whole point:

1. The warning can't corrupt machine-readable output. It goes out via cmd.PrintErrln, so with the bad config still in place octopus config list -f json 2>/dev/null prints clean JSON, and the warning shows up only under 2>&1 1>/dev/null.
2. No other startup path reads the config value unvalidated. ConfigOutputFormat is only SetDefault plus the config file (pkg/config/config.go:32) — it is not in bindEnvironment, so there is no OCTOPUS_*-shaped way back in, and viper.InConfig consults the file map only. No subcommand defines its own PersistentPreRunE (grep for it finds only root.go), so no command can skip the resolution and reach the raw value.
3. Valid-but-oddly-spelled config values aren't thrown away by the new warning. IsValidOutputFormat lowercases and the check trims, so "JSON", " table " and "Basic" in the config file are honoured rather than warned about. Confirmed by running those three through the binary.

Unit coverage: TestNewCmdRoot_PreRunWarnsRatherThanFailingForAnUnsupportedConfigFileValue feeds the global viper a {"outputformat":"xml"} config file, drives the real PersistentPreRunE, and asserts no error, a resolved table, and the warning on stderr. Disabling the new config-value branch fails it with Received unexpected error: unsupported output format 'xml', so it pins the lockout rather than the wording. TestResolveOutputFormat_AnExplicitFlagStillWinsOverAnUnsupportedConfigFileValue covers flag-beats-bad-config.

Residual, stated plainly: config set's own rejection has no unit test. The set package has no test file and setRun writes to the real config path via config.EnsureConfigPath(), so testing it needs either a filesystem seam or an injected viper. I only exercised it through the binary (octopus config set OutputFormat xml exits 1 and leaves the file untouched). Happy to add the seam if you want that covered here rather than in a follow-up.

Blast radius on the disclosed --no-prompt → basic change: it fires on every CI run, not just explicit --no-prompt.

ConfigNoPrompt is bound to the CI environment variable (pkg/config/config.go:58), and GitHub Actions, GitLab CI, CircleCI, Travis etc. all set CI=true. So this branch changes the default stdout of every command in essentially all CI pipelines from table to basic — anyone parsing/snapshotting table output breaks, without ever having passed --no-prompt. The PR description flags this as needing a call; noting here that the trigger is broader than the flag name suggests, which argues for either the delete-the-dead-branch option or a BREAKING CHANGE footer if kept.

The finding holds, and it's worse than "broader trigger": on main this branch is unreachable, so keeping it is a new behaviour change rather than a fix to an existing one.

What I confirmed:

• bindEnvironment binds ConfigNoPrompt to constants.EnvCI (pkg/config/config.go:58), so CI=true — set by GitHub Actions, GitLab CI, CircleCI, Travis, Buildkite, Azure Pipelines — resolves noPrompt true with no --no-prompt anywhere. Reproduced with the built binary: CI=true octopus config list against a config with no OutputFormat prints the basic one-value-per-line form; the same command without CI prints the KEY/VALUE table.
• On main the guard is if v, _ := cmdPFlags.GetString(FlagOutputFormat); v == "" while the flag is registered with a constants.OutputFormatTable default (root.go:95 on main), so v is never empty and the basic write never happens. --no-prompt has therefore never changed the output format on main. Deleting the branch is a no-op against shipped behaviour; keeping it changes default stdout for every command in essentially every pipeline.

So the choice isn't "fix vs. don't fix", it's "introduce this now or not". My read is that table → basic is not a safe default flip for CI: it's a format change on stdout for scripts that never opted into anything, and the people affected are exactly the ones most likely to be parsing or snapshotting it. Against that, basic is arguably what a non-interactive caller wants, and the --no-prompt flag has advertised itself as the non-interactive switch all along.

For now 66d4de8 only writes the $CI trigger down in a comment next to the branch, so whoever reads it next isn't surprised. I have deliberately not made the call.

Open question: delete the case noPrompt: basic branch in this PR so the format stays table everywhere and this PR is a pure bug fix — or keep it and add a BREAKING CHANGE: footer naming $CI, not just --no-prompt, as the trigger? If you want it kept, do you also want it narrowed to an explicit --no-prompt/config set NoPrompt (i.e. not the $CI env binding), which would keep the flag honest without touching CI defaults?

Reuse/dead code: this error string now exists in three places, and this change makes two of them (plus three fallbacks) unreachable.

The identical message lives in pkg/output/print_resource.go:62 and pkg/output/print_array.go:68; with validation centralized here their default: usage-error cases can no longer be hit, and the outputFormat == "" viper fallbacks in print_resource.go:20, print_array.go:20, and pkg/cmd/config/list/list.go:56-58 (plus PrintResource's case constants.OutputFormatTable, "") are now dead. Worth either deleting the dead branches in this PR (no output-shape change, unlike the 14 holdouts) or hoisting the message next to constants.IsValidOutputFormat so the copies can't drift.

Actioned in dc1d7ca — I took the second of the two options: the message is now constants.UnsupportedOutputFormatMessage(outputFormat), sitting directly under constants.IsValidOutputFormat, and root.go, print_resource.go:59 and print_array.go:65 all call it. Wording covered by TestUnsupportedOutputFormatMessage. One correction to the comment: the string was in three places but "" was only in two of them — the print_resource.go and print_array.go copies were byte-identical, root.go's was new in this PR.

I did not delete the dead branches, and that is a residual rather than a disagreement. To be precise about what is still there and why I read it as unreachable:

• print_resource.go:17-20 and print_array.go:17-20 — the outputFormat == "" → viper.GetString(ConfigOutputFormat) fallback
• print_resource.go:41 / print_array.go:45 — case constants.OutputFormatTable, ""
• print_resource.go:59 / print_array.go:65 — the default: usage error
• pkg/cmd/config/list/list.go:55-58 — the same "" fallback

Unreachable because the flag is a root persistent flag with a non-empty default, the pre-run now always resolves it to one of the three valid values, and no subcommand overrides PersistentPreRunE (grep finds it only in root.go), so nothing can skip the resolution. I also checked no test calls output.PrintResource/PrintArray directly with a hand-rolled command — grep for either name across *_test.go returns nothing — so there is no test-only caller keeping them alive either.

Two reasons I left them:

1. PrintResource/PrintArray are exported and take an arbitrary *cobra.Command, so the default: case is the only thing standing between a future command built outside the root tree and printing nothing at all while exiting 0 — which is the failure mode this PR exists to kill. Keeping a loud error at the leaf seems worth more than the line count.
2. The "" viper fallbacks are the one bit I'd actually argue for deleting, because they read the unvalidated config value — the exact soft-brick vector from the other thread — and would reintroduce it if they ever fired. But deleting them means the case ..., "" arms go too, and that widens the diff into two files the PR otherwise only touches for the message hoist.

Open question: want me to delete all four in this PR (it's ~10 lines and no reachable behaviour change, per the above), or leave the leaf-level belt-and-braces and do it as a separate cleanup alongside the 14 hand-rolled-switch holdouts?

Minor, same line: %s is unquoted, so octopus config list -f "" prints unsupported output format . Valid values ... — and "Defaults to table" is no longer always true once this PR lands (with --no-prompt/CI the default is basic).

Actioned in dc1d7ca, taking the suggestion verbatim — quotes around %s, "Defaults to table" dropped. It landed in constants.UnsupportedOutputFormatMessage rather than inline, since the same sentence was in three places (see the sibling thread), so the two pkg/output copies picked up the rewording at the same time.

Confirmed the -f "" case reads properly now, against the built binary:

$ octopus config list -f ""
unsupported output format ''. Valid values are 'json', 'table', 'basic'

Pinned by TestUnsupportedOutputFormatMessage, which asserts the exact string for the empty-value case so the quotes can't be dropped again.